Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add: sqlmesh key_metrics model #2584

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

Jabolol
Copy link
Contributor

@Jabolol Jabolol commented Dec 4, 2024

This PR closes #2268. WIP.

Copy link

vercel bot commented Dec 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kariba-network ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 11, 2024 2:44pm
oso-www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 11, 2024 2:44pm

Copy link

@oso-prs oso-prs bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved! please merge responsibly 😄

@oso-prs
Copy link

oso-prs bot commented Dec 4, 2024

Test deployment unnecessary, no dbt files have been changed.

@ravenac95
Copy link
Member

ravenac95 commented Dec 11, 2024

These changes are good! I actually had a slightly different idea but I think we should merge this first as this will work and pragmatically it makes the most sense. I think this depends on some docs and clean up that I need to do.

Here's what I was hoping:

I was hoping though that we could just write a single metric model. These changes work but my vision for this would definitely require a deeper refactor of the factory. Here's the idea:

  1. A single model for say "commits" is written like the current commits.sql
  2. From this single model we can derive the following style of metrics:
    • Time Aggregations: commits_to_{entity}_daily, commits_to_{entity}_weekly, commits_to_{entity}_monthly, etc.
    • Rolling Window Aggregations: commits_to_{entity}_over_{window}_{unit}
    • Key Metrics: commits_to_{entity}_over_all_time

This would then instead look like this in metrics_factories.py:

        "commits": MetricQueryDef(
            ref="commits.sql",
            time_aggregations=["daily", "weekly", "monthly"],
            rolling=RollingConfig(
                windows=[30, 90, 180],
                unit="day",
                cron="@daily", 
            ),
            include_all_time=True,
        ),

If we do this we would need to change a bit of how the metrics_peer_ref works (I believe)

Sooo....

I will make a follow on issue for this :), but I think we can wrap up anything needed with the current PR and merge it!

@ravenac95
Copy link
Member

ravenac95 commented Dec 11, 2024

Oh also, I'm going to start writing an issue as a design doc for semantic models in sqlmesh. If we did that, then all of this is moot as we could handle that automatically (I think)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Key metrics model in sqlmesh
2 participants